-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Invalidate sources that depends on @inline
methods
#1310
Conversation
No idea whether getModifiers is a hotspot or not. But it only takes a few extra lines anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Hi Eugene! I was asking users on Scala discord about whether this issue bothers Scala users in practice. The feedback I got is that basically everyone already has setup to only use optimizer when producing release binary. Hence this issue do not actually impact anyone. Should we instead not proceed with the PR and instead close the two linked issues sbt/sbt#4125, #537 as won't fix? |
Since the change itself is relatively small, I see no reason we should reject a good change. While it's true, that once you start using optimizer many things would break so people have turned from past injuries to turn them off during normal development, but perhaps changes like this would gradually improve the situation. |
This PR looks right and is an improvement. However in the vast majority of cases methods are not inlined because they carry the |
I had never worked with optimizer, but I think it would be possible for optimizer to provide information for each inlining that took place (caller & callee, along with the class name caller and callee belongs to) and store them in If that solution is indeed reasonable, we can then decide whether we should implement the solution in a follow up PR. |
It's a little confusing that |
The inliner can already produce a log, so adding the right calls to record dependencies should be doable.
But I'm not convinced it's worth the effort. Enabling the optimizer in development is discouraged as it increases compilation times. How about issuing a warning when zinc performs an incremental compilation (not all sources) with the optimizer enabled? |
This PR fixes sbt/sbt#4125, #537.
Issue
When
@inline
method implementation changes, the public API of@inline
method does not change, hence Zinc does not invalidate sources that depends on@inline
method.Solution
The behaviour of
@inline
method is similar to macro methods. Hence, we can mark@inline
method as macro and reuse existing Zinc invalidation logic for macro invalidation.TODOs
@inline
. To handle that case, we need to follow -opt:l:inline breaks incremental compilation #537 (comment) and drop call-site@inline
at compiler side.@inline
annotation, compiler may still sometimes inline a method. The compiler keep a log of inlined methods, so in a follow up PR, we can utilize that information instead of directly checking@inline
annotation.